visibility design and discussion#187
Conversation
|
This looks very solid to me and would definitely bring a big usability gain to libraries like lygia. The one thing that doesn't seem be discussed in much detail is
It seems like the docs here don't explain why this approach is chosen over an
Personally I don't have a strong opinion either way, but there is lots of prior art for the second option (most JS package systems, Python, Rust, Lua) while I can't think of anything that has this strict root/non-root separation off the top of my head. |
|
I believe that we should be able to dig up the Rust recommendations for that. IIRC, Rust discourages the For example, if I have 3 tabs open in VSCode and they all say |
|
Also quick and very inconvenient note: |
|
Without wildcards, resolving an import means going to the right module and finding the item. With wildcards, resolving an import means looking through a set of modules to find the right item. With wildcard re-exports, resolving an import means looking through a set of modules recursively to find the right item. And since modules can have cycles, one now needs to keep track of which modules have been visited during import resolution. In the language server, this is a fair amount of extra complexity. This particular feature also, I believe, interacts really badly with macros or other compile time code generation. Rust's import system ends up being a very complex fixed point finding algorithm which may sometimes simply fail. (I think it also interacts with parameterized modules. Since wildcard reexports is the missing piece to make import statements recursive. If imports can cause any sort of computations, which compile time generated code does, and which parameterized modules do, then one gets the halting problem. Or so the story goes.) Usually I wouldn't object to complexity too much. |
Not sure, always interesting to look around tho! Here I was only filling the gap where our current scheme doesn't allow something at the root level. I think small libraries especially might like to enable their users to It'd be a separate issue, but your comments about consistency... I suppose we could revise the existing rules so that |
wat!? good catch. Perhaps |
Worth thinking about. I like the idea of implementing the needed features w/o gilding the lily too much. The cycles/complexity concerns seem heavy for a feature that isn't must-have. And foreclosing options for parameterized modules would be unfortunate. Also I realize that the PR doesn't address some related issues:
I'm thinking best would be to pull wildcard re-exports from this PR. Side note, if we go with the publish tool route as planned with #183, I suppose we could also expand wildcard re-exports like we're planning for wildcard imports. That would help for the cross package cases. |
|
pushed a draft
|
|
We could also go with
Also, what does private mean? Rust goes all in on the hierarchical module system and says
|
I like this option too, a bit more cryptic but if perhaps best if we prefer keywords to attributes.
I like I suppose we could do
just the module, no hierarchy. we could expand hierarchically later, it's a nice idea. But I think of most WESL programs as smallish, so perhaps unnecessary complexity, better to defer. |
|
For the
|
Ah, that was my first instinct too. I like modularity. But I've come to think that package by default for WESL is better. See |
|
Also interesting tidbit: The other style is the global style, where I use 3 access modifiers. Public, internal (protected) and private. And apparently some of the very prolific Rust contributors, including epage who maintains a bunch of extremely well written libraries, vastly prefer the global style. So we are on the right track! https://www.reddit.com/r/rust/comments/1k5yv6p/two_ways_of_interpreting_visibility_in_rust/ |
|
More from Rust land:
They also include statistics which says that |
|
And from Kotlin lang: They went with public by default, only to realize that it might have been a mistake. So defaulting to a more restrictive choice like "internal" seems wise |
|
Note from Java: |
|
We really should ask the WGSL committee regarding the |
|
I dug into it and asked the WebGPU commitee gpuweb/gpuweb#6264 I couldn't quite figure out where it got lost though. It seems to have happened quite a while ago, judging from Naga's source code |
|
I talked to a friend regarding this. |
|
And Jim Blandy pointed out that Rust doesn't have a private keyword. |
|
I asked the Slang team! |
| `ident`s must not be current WGSL keywords. `ident`s also must not be | ||
| current WESL keywords: `as`, `import`, `module`, `package`, `super`, or `self`. | ||
| `ident`s must not be current WGSL keywords. `ident`s also must not be | ||
| current WESL keywords: `as`, `import`, `module`, `package`, `private`, `public`, `self`, or `super`. |
There was a problem hiding this comment.
Remove private from this list, it's a context sensitive keyword
|
|
||
| To get an absolute path to a module, one follows the algorithm above. In step 1, one takes the known absolute path of the `super` module, or the package. | ||
| The absolute path of the `super` module is always known, since the first loaded WESL file must always be the root module, and children are only discovered from there. | ||
| The steps above resolve a path; whether the referencing module may then use the result is governed by [visibility](Visibility.md). |
There was a problem hiding this comment.
How does this interact with wildcards?
Is this a name clash?
import foo::*;
import bar::*;
const B = A;
when
// foo.wesl
private const A = 3;
// bar.wesl
public const A = 3;
Or is visibility more intertwined with name resolution?
There was a problem hiding this comment.
good question, I think it should be only be a clash if both are visible.
| [WGSL recursive descent grammar](https://www.w3.org/TR/WGSL/#grammar-recursive-descent) | ||
| for WGSL's full attribute production. | ||
|
|
||
| ### Referring to less-visible declarations |
There was a problem hiding this comment.
Is there a reason why this should be allowed at all?
There was a problem hiding this comment.
Authors can write a fn that makes and returns a struct with private visibility. API users can pass the struct back to another fn, but can't build one them themselves w/o going through the API. That might be nice for the author, maybe they want to track every struct built in a table or something and don't want users to be able to able to cheat around their intended makeThing() api.
Requiring that every struct mentioned in a public fn also be public is reasonable, but less flexible for API authors. Other languages undoubtedly differ on this. I was just scanning through Rust RFC 2145 which makes it sound like Rust had a strict requirement and then changed to warn/lint. Go allows it but has a lint. TypeScript allows it I think.
There was a problem hiding this comment.
Couldn't one achieve the same result by making one of the struct fields private? Then the struct is also not constructible for the API users.
I don't fully understand why Rust relaxed their rules, from the sounds of it they started out with a suboptimal version of "no private in public".
| name the less-visible type to declare a variable, import it, or construct a new | ||
| value. | ||
|
|
||
| WESL publishing tools should warn when an `@public` declaration mentions a |
There was a problem hiding this comment.
What about other WESL tools like the language server?
There was a problem hiding this comment.
good point, other tools like the lang server should warn too.
| as MeshA;` alongside `... as MeshB;`) just adds reachable names; all of them | ||
| resolve to the single declaration. | ||
|
|
||
| ### Only public items can be re-exported |
There was a problem hiding this comment.
Why restrict re-exporting to @public? I find it quite useful within a package as well, once the package crosses a certain size threshold.
There was a problem hiding this comment.
simplicity, could add later I agree
There was a problem hiding this comment.
I think it'd be simpler to implement if we have the internal variant as well. The way it's currently phrased with @public means that I need to add quite specific checks for the restrictions.
(This is very speculative of course. My point is mostly that I think we can safely allow more here, without adding notable implementation complexity.)
| `@public import`s it. | ||
|
|
||
| An override with a default value that is absent from the pipeline-visible API | ||
| silently degrades to a constant: the linker bakes in the default and the host |
There was a problem hiding this comment.
This is technically not possible. Consider the following
public override bar = 3;
private override foo = bar;
Then if we downgrade foo, we get
override bar = 3;
const foo = bar; // error, this is an override-expression, not a const-expression
| `@private` item as `@public` is a hard error. | ||
|
|
||
| The reason is that the visibility attribute on a declaration is meant to be a | ||
| local contract. An author looking at a *package* item should be able to rely on |
There was a problem hiding this comment.
I really like this "local reasoning" argument. 👍
| immediately before the declaration keyword, so a reader scanning down the left | ||
| margin can find it without hunting through the attribute list. | ||
|
|
||
| ### Alternatives considered |
There was a problem hiding this comment.
agreed, will change back. Seems pretty parseable.
| relaxation would let the root module `@public import` *package* items from the | ||
| same package, since the root defines the package's external and pipeline-visible | ||
| API. The cost is loss of orthogonality: what `@public import` accepts would | ||
| depend on whether the importer is the root. |
There was a problem hiding this comment.
Ah, so basically I cannot do public import package::some_plain_wgsl_file::fragment_shader;. Because the fragment_shader in the plain WGSL file isn't public.
There was a problem hiding this comment.
But I should be able to do internal import package::some_plain_wgsl_file::fragment_shader;. After all, package-level items are pipeline-visible.
| mark those items explicitly, even authors who don't otherwise care about | ||
| encapsulation. Package as the default makes encapsulation discipline available | ||
| to projects that want it (via `@private`), without forcing typical small | ||
| projects to do the extra work. |
There was a problem hiding this comment.
I don't think this argument holds up. Pretty much every host language (C, C++, Rust, Typescript, ...) already makes the authors do this work. And my the dream shading language would be whatever host language I use.
So using the same patterns across my codebase is less mental overhead for me. Instead of having to remember "wait, well written WESL means that I have to write private more often"
| to projects that want it (via `@private`), without forcing typical small | ||
| projects to do the extra work. | ||
|
|
||
| * **WGSL files can be imported without modification.** WGSL has no visibility |
There was a problem hiding this comment.
I sort of agree with this one. It should be possible to incrementally adopt wesl.
But at the same time, I'd also like to note that that existing WGSL code has no imports and is not always designed for it.
If I were to add visibility to WGSL itself, I would likely choose an option where one has to add visibility markers before being able to use the code in other files. This is analoguous to what Javascript did with the export keyword.
In fact, as an imaginary WGSL (not WESL) standard writer, I would argue that allowing arbitrary imports is a footgun. Existing WGSL code has been written without a module system in mind. So there are no encapsulation boundaries anywhere. I would start accessing all the right and all the wrong things, without the language steering me in the correct direction.
With my IDE hat on, I would agree. If I add "allow imports from anywhere" into any of my existing WGSL codebases, I'd get a hundred silly recommendations. Often I'd get 5 separate recommendations for the same type, because I copied it 5 times. And worse yet, of those 5 times, 2 of them would be subtly different, due to reasons like gpuweb/gpuweb#4574
| principle, and shader source loaded as a string (without a file extension) | ||
| would still need some other way to mark the dialect. | ||
|
|
||
| * **Root module entry points would need explicit markers.** A root module's |
There was a problem hiding this comment.
This is a solid argument. So "private by default" with a file-level private would immediately break all WGSL files.
Which means that the WGSL standards commitee could not choose this option without introducing a breaking change to WGSL.
In terms of other options:
- For the entrypoints and bindings, there is the option of saying "
@fragmentimpliespublic" and "@groupimpliespublic". But that feels weirdly inconsistent with the rest of the language. And this rule fails to cover overrides. - Root modules with a different behaviour seem weird. Having a consistent language means fewer things that I have to remember.
privatecould simply not affect pipeline visibility. But then we break encapsulation in a weird way.privatecould be much less strict, similar to Rust. Rust made the observation that file level private is often still too strict.
| toward package as the default: | ||
|
|
||
| * **Encapsulation gains are smaller at WESL scale.** A typical WESL project is a | ||
| handful of files: a vertex/fragment pair, a compute shader, perhaps a few |
There was a problem hiding this comment.
I'm not sure if I agree with the assumption here. I'm more familiar with game development, where a typical shader codebase is
- a few files that I wrote. These are indeed somewhat small. But they are also very standalone, due to the nature of things. In fact, while many big game engines (e.g. Unreal Engine) have "material functions", it is kind of rare for me to import code from another shader that I wrote.
- a massive amount of game engine shader code that I rely on. I import a few types from here and the rest is a black box
And I suspect that the same is true on the web, since frameworks like Babylon.js and Three.js are quite popular.
So targeting small shader projects that are not already a part of a game engine is, I think, targeting an overly small part of the userbase.
TLDR
public(visible to any package),private(declaring module only), and package (same package, the unmarkeddefault).
public importre-exports the imported names under thecurrent module's path;
public import path::*does the same for a wildcard(re-uses
@wildcardablediagnostics).API: entry points, resource variables, and pipeline-overridable constants
reach the host only if the root module declares them or
public imports them.package.wesl. The file backing a package's top-level module, so itsitems are reachable as
<package>::item(e.g.,import wgsl_test::*). Oneplace a library can put a re-export prelude.
Why these four things are covered:
import wgsl_test::*;(Note that VisibilityLanguages.md is discussion background, it'll be removed before merging.)
Would resolve:
(largely covered already; this PR fills in the re-export piece)
Related